[core] port matrix and antialiasing cleanup #7444

ansis · 2016-12-15T00:26:04Z

This cleans up a bunch of matrix and antialiasing code.

removes the confusing altitude and replaces it with fov

fixes text blurriness at non-default FOVs

removes the hack added in [core] Rationalize Resource initialization #3740 and adds a different fix

makes pitched line antialiasing clearer and removes lineStretch and lineAntialiasingMatrix

TODO:
I removed altitude from CustomLayerRenderParameters and added fieldOfView. Is this considered an external interface / breaking change? Should I remove it from the platform bindings as well? Or should I calculate it and add it? The old altitude value is just 0.5 / tan(fov / 2) * height

@jfirebaugh

mention-bot · 2016-12-15T00:26:05Z

@ansis, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @1ec5 and @incanus to be potential reviewers.

jfirebaugh · 2016-12-15T01:39:48Z

I removed altitude from CustomLayerRenderParameters and added fieldOfView. Is this considered an external interface / breaking change?

It's a breaking change to an undocumented API, so kind of a grey area. We're already breaking this API for iOS 3.2.0, and could conceivably squeeze this change into that release. However, Android 4.2.0 shipped today, and it's a breaking change there as well.

cc @1ec5 @cammace

1ec5 · 2016-12-15T02:26:37Z

We're going to merge release-ios-3.4.0 back to master tomorrow following today's release of iOS SDK v3.4.0-beta.5. The release branch contains some significant changes to custom layers (#7250) that would conflict with this branch if you were to update the existing custom style layer API.

Of note, the drawing parameters are now contained in a struct, which means we could simply add fieldOfView alongside the (meaningless, unused) perspectiveSkew field (which maps to altitude in mbgl). But I'd actually prefer that we go ahead and replace perspectiveSkew, since we haven't officially released our other backwards-incompatible changes yet.

1ec5 · 2016-12-15T02:33:15Z

Meanwhile, I can remove the perspectiveSkew field on the release branch.

1ec5 · 2016-12-15T02:35:33Z

The old altitude value is just 0.5 / tan(fov / 2) * height

Wasn't it hardcoded to 1.5? This is why I figured it would be safe to remove without much fuss.

ansis · 2016-12-20T23:43:14Z

I replaced altitude with fieldOfView in this ios stuff. I think this is safe to break considering it's an undocumented api and an argument probably no one was using.

All this needs is review

jfirebaugh

I agree. Can you squash the last commit with 6aa1f92 so that there aren't and failing intermediate commits?

1ec5 · 2016-12-21T00:54:16Z

src/mbgl/map/transform_state.hpp

@@ -109,7 +110,7 @@ class TransformState {
    double x = 0, y = 0;
    double angle = 0;
    double scale = 1;
-    double altitude = 1.5;
+    double fov = 0.6435011087932844;


Oh, any chance that you could replace this with an expression or add a comment to the same effect?

This comment should also be updated to point to this field.

ported from -js: c52a09639ceeeb809cd837360993df9c45b45420 This replaces a hardcoded shader approximation with a more correct, FOV-dependent one. `gl_Position.w - 0.5` is replaced with `gl_Position.w / tan(fov)` The `/ tan(fov)` is handled by multiplying `1 / tan(fov)` into `u_gamma` outside the shader. I think `- 0.5` was just chosen as something that looked right for the default fov.lease enter the commit message for your changes. Lines starting

ported from -js: eb6c6596c6a7a61363d30356674e0002153b1d19 `altitude` was a terribly-named variable that was used to indirectly control the fov. This should eliminate some confusion. `altitude` was equivalent to `cameraToCenterDistance / height`

ported from -js: ef5582dd3bc5c15a3112e875ed66494dab8e9d0b Project the extrusion and compare it's projected pixel length with the actual pixel length and adjust antialiasing accordingly. The previous approach calculated the adjustment much more indirectly and had no intuitive explanation.

ported from -js: 0b5520fa5ab2a4659d80dcffa8b035a0d84fe1ca This should fix the issue behind #2270 and remove the need for the hack added in #3740.

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574.

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574, and #7770. Also corrected the blurb about #7711.

ansis changed the title ~~port matrix and antialiasing cleanup~~ [core] port matrix and antialiasing cleanup Dec 15, 2016

ansis force-pushed the matrix-cleanup branch 3 times, most recently from b837ab5 to b2772e9 Compare December 20, 2016 22:13

ansis requested a review from 1ec5 December 20, 2016 23:43

jfirebaugh approved these changes Dec 20, 2016

View reviewed changes

ansis force-pushed the matrix-cleanup branch from 6617ff8 to c598672 Compare December 20, 2016 23:53

ansis removed the request for review from 1ec5 December 20, 2016 23:54

1ec5 approved these changes Dec 20, 2016

View reviewed changes

1ec5 reviewed Dec 21, 2016

View reviewed changes

1ec5 mentioned this pull request Dec 21, 2016

Omit perspectiveSkew from drawing context struct #7501

Merged

ansis force-pushed the matrix-cleanup branch from c598672 to dc7047a Compare December 21, 2016 23:05

ansis added 3 commits December 21, 2016 15:11

[core, ios] replace altitude with fov

cc9bb97

ported from -js: eb6c6596c6a7a61363d30356674e0002153b1d19 `altitude` was a terribly-named variable that was used to indirectly control the fov. This should eliminate some confusion. `altitude` was equivalent to `cameraToCenterDistance / height`

[core] fix matrix z range and remove hack

d0a0d81

ported from -js: 0b5520fa5ab2a4659d80dcffa8b035a0d84fe1ca This should fix the issue behind #2270 and remove the need for the hack added in #3740.

ansis force-pushed the matrix-cleanup branch from dc7047a to d0a0d81 Compare December 21, 2016 23:11

jfirebaugh merged commit 2cf238f into master Dec 21, 2016

jfirebaugh deleted the matrix-cleanup branch December 21, 2016 23:46

1ec5 added a commit that referenced this pull request Jan 16, 2017

[ios, macos] Updated changelogs

882fd79

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574.

1ec5 mentioned this pull request Jan 16, 2017

Update iOS, macOS changelogs #7731

Merged

1ec5 added a commit that referenced this pull request Jan 19, 2017

[ios, macos] Updated changelogs

5c144a2

Updated changelogs to mention #7446, #7356, #7465, #7616, #7445, #7444, #7526, #7586, #7574, and #7770. Also corrected the blurb about #7711.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] port matrix and antialiasing cleanup #7444

[core] port matrix and antialiasing cleanup #7444

ansis commented Dec 15, 2016 •

edited

Loading

mention-bot commented Dec 15, 2016

jfirebaugh commented Dec 15, 2016 •

edited

Loading

1ec5 commented Dec 15, 2016

1ec5 commented Dec 15, 2016

1ec5 commented Dec 15, 2016

ansis commented Dec 20, 2016

jfirebaugh left a comment

1ec5 Dec 21, 2016 •

edited

Loading

1ec5 Dec 21, 2016

[core] port matrix and antialiasing cleanup #7444

[core] port matrix and antialiasing cleanup #7444

Conversation

ansis commented Dec 15, 2016 • edited Loading

mention-bot commented Dec 15, 2016

jfirebaugh commented Dec 15, 2016 • edited Loading

1ec5 commented Dec 15, 2016

1ec5 commented Dec 15, 2016

1ec5 commented Dec 15, 2016

ansis commented Dec 20, 2016

jfirebaugh left a comment

Choose a reason for hiding this comment

1ec5 Dec 21, 2016 • edited Loading

Choose a reason for hiding this comment

1ec5 Dec 21, 2016

Choose a reason for hiding this comment

ansis commented Dec 15, 2016 •

edited

Loading

jfirebaugh commented Dec 15, 2016 •

edited

Loading

1ec5 Dec 21, 2016 •

edited

Loading